Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Onfleet node #2593

Closed
wants to merge 17 commits into from
Closed

Onfleet node #2593

wants to merge 17 commits into from

Conversation

jamesliupenn
Copy link
Contributor

@jamesliupenn jamesliupenn commented Dec 22, 2021

Preparation

  • Set up your editor for code formatting (indentation, new lines, linting). If you use Visual Studio Code, you can use the TSLint extension for linting.
  • Get credentials (e.g., Client ID, Client Secret, API key, user login, user password, website URL) for the service you are building a node for.

Development

  • Open a pull request as early as possible with WIP in the pull request title. (We're doing this in one big PR 😆 )
  • If you are creating a node requested by a community member, make sure to comment on the feature request in the community forum (opens new window).
  • Ensure complementary operations to each resource (e.g., create, delete) have been added.
  • Ensure the node works with multiple items via one input.
  • Ensure the parameters have the correct type.
  • Mind the defaults: if the service has a default as true, keep it as true. Changing default values can break the existing workflows of the users.
  • Check if the node disposes of everything properly, in particular, if connections were properly closed.
  • Check your code using Nodelinter to ensure a clean lint before submitting your pull request

Testing

  • Test create and update operations with all fields/operations.
  • Test the continueOnFail option with a Function node. (For example, a Widget node has a GET operation that takes a widgetId and returns information on the widget. To test that the workflow continues on fail, set the Widget node to continue on fail, create a Function node, return a valid and an invalid widgetId, connect the Function node to Widget node, and run the workflow. The Widget node should show two items: one with information on the widget and another one with the error from having passed an invalid ID.)

Code formatting

  • Ensure the branch lints cleanly by running npm run lint.
  • Ensure the indentation is correct. Check this in the editorconfig.
  • Ensure there are no extra spaces. Check this in the editorconfig.
  • Code comment dividers inside if-branches.
  • Use "create/delete" verbs for operations, except for tags, where you should use "add/remove". (We still opt to use create in several cases to be uniform with our API documentation)

Errors and Outputs

  • Ensure empty API responses return { success: true }.
  • Ensure the error responses are handled and displayed correctly (e.g., malformed requests, requests with invalid credentials) and use the current format. You can check this by making failing requests to the API.
  • Check if the response can be simplified and add a simplify function (e.g., SecurityScorecard node). (Not needed in our case)
  • Ensure the response from Create is consistent with Get.
  • Ensure the response from Get All is consistent with Get.

Presentation

  • Ensure the primary menu contains only required parameters.
  • Ensure a JSON object is not shown in a single column in Table view. (Nested JSON)
  • Make sure all GetAll operations have the fields return and limit.
  • Ensure the property subtitle is set.
  • Make sure the pagination (if any) is working correctly. Set Limit 1.

Writing

  • Ensure all descriptions are correct and end with a period.
  • Ensure that most descriptions exist, excluding redundant ones.
  • Ensure IDs in displayNames are capitalized (i.e.: "IDs", not "ids" or "Ids").
  • Ensure that IDs, if multiple, have descriptive qualifiers.
  • Ensure the name property in description in the node class is written in camelCase.
  • Ensure the file name and the Class name are identical.

Branding

  • Ensure the name of the service is written correctly (e.g., "GitHub" not "Github"). If the node is a trigger node, ensure it is named as such, by adding "Trigger" after the service name (e.g., "Trello Trigger").
  • Ensure the logo is either a PNG or SVG, ideally the latter. Vecta is a good website to find SVGs of different applications.
  • If the logo is an SVG, ensure the canvas is a perfect square. If the logo is PNG, ensure it is 60x60 pixels and compressed.
  • Ensure the border color of the node matches the branding of the service.

Nice-to-haves (optional)

  • Add handler for continueOnFail. This feature is included in some of the newest nodes (e.g Lemlist node) to continue the workflow even if the node's execution fails.
  • Remove required: false and description: '' in the node descriptions (e.g., Lemlist node).
  • At call site, specify first body and then qs.
  • At call site, prepend the endpoint with slash / (e.g., "/campaign").

Santiago Botero Ruiz and others added 6 commits December 13, 2021 20:17
Added Onfleet nodes for working with different endpoints like:
organizations, administrators, workers, hubs, teams, destinations, recipients,
containers and webhooks.
INT-441: Add Onfleet nodes to n8n
feat: added name field to onfleet trigger node
@CLAassistant
Copy link

CLAassistant commented Dec 22, 2021

CLA assistant check
All committers have signed the CLA.

@ivov ivov added node/new Creation of an entirely new node community Authored by a community member labels Dec 23, 2021
@jamesliupenn
Copy link
Contributor Author

@ivov hi there, do you know what's the typical lead time to get a new node added? We're anticipating this release and hope to get an estimate of when this will be live.

@janober
Copy link
Member

janober commented Jan 14, 2022

Thanks a lot for the contribution @jamesliupenn. The node is one of the next ones that will get reviewed. So should not be much longer.

@jamesliupenn
Copy link
Contributor Author

Thanks! I realized we missed a few operations so we'll be making some updates tomorrow

Copy link
Contributor

@RicardoE105 RicardoE105 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got reviewed. Let me know if you have questions about any comments. @jamesliupenn

body: any = {}, // tslint:disable-line:no-any
qs?: any, // tslint:disable-line:no-any
uri?: string,
headers: IDataObject = {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header is a parameter but it's never used in the function?

const apiError = error as IDataObject;
const { message: messageError } = apiError.error as IDataObject;
if (messageError) {
const { message = '', cause = '' } = messageError as IDataObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you simply pass the error object to the NodeApiError?

value: 'admins',
},
{
name: 'Containers',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resources names and values need to be singular.

value: 'webhooks',
},
{
name: 'Hubs',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resources need to be ordered alphabetically

description: INodeTypeDescription = {
displayName: 'Onfleet Trigger',
name: 'onfleetTrigger',
icon: 'file:Onfleet.png',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are two different images? Onfleet and Onfleet@2x? Can you please optimize the png using https://tinypng.com/.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or preferably use an SVG logo instead.

displayName: 'End',
name: 'end',
type: 'dateTime',
default: Date.now(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set default to ''


export const workerFields = [
{
displayName: 'ID',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worker ID

vehicleColorField,
],
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title case

],
},
{
displayName: 'Additional fields',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Fields

{
displayName: 'Additional fields',
name: 'additionalFields',
type: 'collection',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analytics fields need to be optional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the collection to Options.

Added team auto-dispatch and driver time estimate endpoints to Onfleet
node
Santiago Botero Ruiz and others added 2 commits January 19, 2022 09:48
@jamesliupenn
Copy link
Contributor Author

We'll be prioritizing this in the ongoing sprint, thanks for the feedback @RicardoE105, we'll keep you posted once we finish developing it internally.

Santiago Botero Ruiz and others added 7 commits January 27, 2022 09:38
added new fixed collections, refactored the code according to comments
made on the n8n pr
added docstrings for new functions addded for formatting the destination
and recipient objects
Refactor according to n8n PR comments
@jamesliupenn
Copy link
Contributor Author

@RicardoE105 this is ready for another round of review as we have resolved the issues you mentioned. Please keep us posted👍👍

@RicardoE105
Copy link
Contributor

@jamesliupenn perfect. Will review it again as soon as I can.

@RicardoE105
Copy link
Contributor

@jamesliupenn had a chance to review again. This time I did a deeper review than last time and found the following. Can you please address these issues? Thanks.

Onfleet review

Admin resource

Admin:getAll

  • Admin:getall missing pagination

Admin:delete

  • Return { success: true } when the operation is successfull

Task resource

  • Do only task:create and do the batching behind the scenes. Batching if available should be the implementation to use.

Task:create

  • Every field that is not required should be under a collection. I was able to create a task only with the destination address, which means, that recipients it’s not required
  • When setting the address you have a collection called “additional fields” that is not necessary. Add them at the same level of the destination address and make them not required. Do the same with Recipient
  • Rename additional task fields to Additional Fields

Task:clone

  • If the operation does not return anything when it ran successfully return { success: true }
  • Rename collection options to additional fields
  • Move what is inside the collection override to the same level the options parameters are

Task:complete

  • If this operation completes the task why would I make the success parameter false?
    Return { success: true } when the operation is successful

Task:delete

  • Return { success: true } when the operation is successfull

Task:getAll

  • Missing pagination
  • From does not seem to be a required parameter. Add It under the collection filter fields
  • Rename filter fields collection to filters
  • Remove Last ID parameter.
  • It does not seem to return the task I created

Task:get

  • Is the short id parameter really needed? Can just detect that behind the scenes and make the corresponding call?

Container resource

  • I would probably only leave the get operation for this resource. Then, move insert and update to another resource called containerTask.

Container:insertTasks

  • I would create different operations depending on the insert type. for example: containerTask:append, containerTask:Prepend.
  • Rename tasks to Task IDs

Hub resource

Hub:getAll

  • Missing pagination

Hub:update

  • Destination should be under update fields

Team resource

Team:auto-dispatch

  • What is road end exactly? an address?
  • Return { success: true } when the operation is successfull

Team:create

  • All the fields marked as required, are not actually required. Everything that is not required should not be at the main level but instead should be moved underneath a collection. In this case, the additional fields collection

Team:delete

  • Return { success: true } when the operation is successfull

Team:getAll

  • Missing pagination

Team:getTimeStimates

  • Dropoff and pickup do not seem to be required parameters. Both need to be under additional fields if this is the case

Webhook resource

  • Is it the webhook resource usufull in the node? I would not say so, since we are already using those endpoints in the trigger. Do you have a use case in mind where you need to create Webhooks dynamically?

Worker resource

Worker:create

  • It does not seem to be working. No matter the input I provide.

worker:getAll

  • Missing pagination

* Fixed some error creating a worker, moving some fields under additional fields collection

* Fixed returned values for delete operations, making some changes for style code

* Added operational error since required property is not working for dateTime fields
@jamesliupenn
Copy link
Contributor Author

jamesliupenn commented Feb 16, 2022

Admin resource
Admin:getAll

  • Admin:getall missing pagination there are no pagination from the Onfleet API endpoint itself, no need for us to paginate

Admin:delete

  • Return { success: true } when the operation is successful FIXED

Task resource

  • Do only task:create and do the batching behind the scenes. Batching if available should be the implementation to use. FIXED now we only do create single task if the input contains only one task's data, batch create if input contains multiple tasks' data. To implement multiple task creation with create single task endpoint, n8n users can opt in to iterate through the input and call create single task multiple times.

Task:create

  • Every field that is not required should be under a collection. I was able to create a task only with the destination address, which means, that recipients it’s not required
  • When setting the address you have a collection called “additional fields” that is not necessary. Add them at the same level of the destination address and make them not required. Do the same with Recipient
  • Rename additional task fields to Additional Fields FIXED

Task:clone

  • If the operation does not return anything when it ran successfully return { success: true } FIXED
  • Rename collection options to additional fields FIXED
  • Move what is inside the collection override to the same level the options parameters are FIXED

Task:complete

  • If this operation completes the task why would I make the success parameter false? There are task completions that have failed, as Onfleet accepts a failed task completion, meaning that delivery was attempted but the recipient didn't receive the delivery due to some circumstances. The success parameter can be set to false.
  • Return { success: true } when the operation is successful FIXED

Task:delete

  •   Return { success: true } when the operation is successful FIXED

Task:getAll

  •  Missing pagination Pagination is using last ID parameter
  • From does not seem to be a required parameter. Add It under the collection filter fields from is required per API documentation and design
  • Rename filter fields collection to filters FIXED
  • Remove Last ID parameter. See pagination comment
  • It does not seem to return the task I created

Task:get

  • Is the short id parameter really needed? Can just detect that behind the scenes and make the corresponding call? FIXED We now detect the length of the string, if short-formatted, accesses the shortId endpoint

Container resource

  • I would probably only leave the get operation for this resource. Then, move insert and update to another resource called containerTask.

Container:insertTasks

  • I would create different operations depending on the insert type. for example: containerTask:append, containerTask:Prepend. We have one operation for task container insertions, managing the different methods in dropdown
  • Rename tasks to Task IDs FIXED

Hub resource
Hub:getAll

  • Missing pagination there are no pagination from the Onfleet API endpoint itself, no need for us to paginate

Hub:update

  •   Destination should be under update fields FIXED

Team resource
Team:auto-dispatch

  • What is road end exactly? an address?
  • Return { success: true } when the operation is successful FIXED

Team:create

  • All the fields marked as required, are not actually required. Everything that is not required should not be at the main level but instead should be moved underneath a collection. In this case, the additional fields collection

Team:delete

  • Return { success: true } when the operation is successful FIXED

Team:getAll

  •   Missing pagination there are no pagination from the Onfleet API endpoint itself, no need for us to paginate

Team:getTimeEstimates

  • Drop Off and Pick Up do not seem to be required parameters. Both need to be under additional fields if this is the case FIXED

Webhook resource

  • Is it the Webhook resource useful in the node? I would not say so, since we are already using those endpoints in the trigger. Do you have a use case in mind where you need to create Webhooks dynamically? This is actually useful in the node, because there are use cases where webhooks operations are done systematically. We want to be as close to the API as possible

Worker resource
Worker:create

  • It does not seem to be working. No matter the input I provide. FIXED This wasn't throwing the right errors to you when you attempted

worker:getAll

  •   Missing pagination - there are no pagination from the Onfleet API endpoint itself, no need for us to paginate

@jamesliupenn
Copy link
Contributor Author

@YokySantiago to finish up the checklist items, @RicardoE105 this is over to you for review

@YokySantiago
Copy link
Contributor

YokySantiago commented Feb 16, 2022

In order to complete what James mentioned

Task:getAll

  • It does not seem to return the task I created. Are you testing with an Onfleet account? Did you set a From value? From field is required for this operation but n8n it's not displaying it as required so, we created a new topic in n8n and in the meantime we added a manual validation. n8n report

Container resource
In this case we don't have container information. This is the meaning of container for us "Containers are an abstraction which describes task assignment. A container is an ordered list of tasks. Organizations, teams and workers all correspond to containers as they can all be assigned tasks." So, when we get information about a container, we need to select the type (Organization, Team, Worker) in order to get the task assignment for the container selected.
So, We would like to keep it all together as a resource

Team:auto-dispatch

  • What is road end exactly? an address?. This field is a select now with some additional option, improving the usability. FIXED

Team:create

  • All the fields marked as required, are not actually required. Everything that is not required should not be at the main level but instead should be moved underneath a collection. In this case, the additional fields collection. FIXED

@RicardoE105
Copy link
Contributor

RicardoE105 commented Feb 18, 2022

@jamesliupenn @YokySantiago I'm trying to test the team resource, but it says "payment is required". Can you provide a test account with access to the team endpoints?

image

Also, to create a container I need a container ID. Where can I get a container ID from?

RicardoE105 added a commit that referenced this pull request Feb 18, 2022
RicardoE105 added a commit that referenced this pull request Feb 18, 2022
@jamesliupenn
Copy link
Contributor Author

jamesliupenn commented Feb 18, 2022

@jamesliupenn @YokySantiago I'm trying to test the team resource, but it says "payment is required". Can you provide a test account with access to the team endpoints?

image

Also, to create a container I need a container ID. Where can I get a container ID from?

@RicardoE105

What endpoint on the teams endpoint are you trying to access? If you are hitting specific endpoints like auto dispatch, you will need that feature enabled on your account and I can help you with that.

Containers are abstractions of task assignments, so if you assign a task to a worker/org/team, there will be a container created. The container IDs are be available via the worker/org/team ID, so basically depending on which entity you are accessing the container for, use that entity ID.

An example would be accessing a team's container, simply use that team ID

@RicardoE105
Copy link
Contributor

Only get the payment required error when using auto dispatch

@RicardoE105
Copy link
Contributor

I created an account with my email: ricardo@n8n.io. If you can enable it for that account would be great. Thanks.

@jamesliupenn
Copy link
Contributor Author

@RicardoE105 you should be on a test account and has the auto-dispatch endpoint enabled now

janober added a commit that referenced this pull request Feb 28, 2022
* feat: added Onfleet nodes

Added Onfleet nodes for working with different endpoints like:
organizations, administrators, workers, hubs, teams, destinations, recipients,
containers and webhooks.

* style: fixed typos, arrays uniformity, unnecesary files

* refactor: changed add to create in comments and labels

* feat: added name field to onfleet trigger node

* feat: added team endpoints to onfleet node

Added team auto-dispatch and driver time estimate endpoints to Onfleet
node

* style: remove dots in descriptions and fixed some typos

* feat: added fixes according to comments made on the n8n PR

added new fixed collections, refactored the code according to comments
made on the n8n pr

* fix: fixed recipient and destination cretion

* docs: added docstrings for format some functions

added docstrings for new functions addded for formatting the destination
and recipient objects

* style: formatting the code according to n8n nodelinter

* fix: typos and better descriptions

* [INT-510] n8n: Address additional problems from n8n code review (#5)

* Fixed some error creating a worker, moving some fields under additional fields collection

* Fixed returned values for delete operations, making some changes for style code

* Added operational error since required property is not working for dateTime fields

* ⚡ Improvements to #2593

* ⚡ Improvements

* 🐛 Fix issue with wrong interface

* ⚡ Improvements

* ⚡ Improvements

* ⚡ Minor improvement

Co-authored-by: Santiago Botero Ruiz <santiago.botero@devsavant.ai>
Co-authored-by: ilsemaj <james.li.upenn@gmail.com>
Co-authored-by: Santiago Botero Ruiz <39206812+YokySantiago@users.noreply.github.com>
Co-authored-by: Jan Oberhauser <jan.oberhauser@gmail.com>
@janober
Copy link
Member

janober commented Feb 28, 2022

Thanks a lot for your contribution. Got merged with #2845

@janober janober closed this Feb 28, 2022
@janober janober added the Upcoming Release Will be part of the upcoming release label Feb 28, 2022
@janober
Copy link
Member

janober commented Feb 28, 2022

Got released with n8n@0.165.0

@janober janober removed the Upcoming Release Will be part of the upcoming release label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/new Creation of an entirely new node
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants